Skip to content

feat: mock SAML 2.0 IdP for testing#1395

Closed
BilalG1 wants to merge 11 commits intodevfrom
pr/saml-mock-idp
Closed

feat: mock SAML 2.0 IdP for testing#1395
BilalG1 wants to merge 11 commits intodevfrom
pr/saml-mock-idp

Conversation

@BilalG1
Copy link
Copy Markdown
Collaborator

@BilalG1 BilalG1 commented Apr 29, 2026

First of 3 stacked PRs adding SAML 2.0 SSO. Stacked above: [#PR_BACKEND] (backend feature) → [#PR_CLIENT] (client + tests).

What this PR adds

  • apps/mock-saml-idp/ — standalone Bun/Express service that acts as a multi-tenant SAML 2.0 Identity Provider for e2e tests and local dev. Listens on port 8115. Each tenant has its own RSA keypair + self-signed cert generated at startup, so one mock service can back many SamlConnection rows.
  • CI workflow — starts `mock-saml-idp` alongside `mock-oauth-server` in `.github/workflows/e2e-api-tests.yaml`.
  • Root scripts — `pnpm start:mock-saml-idp` + `dev:basic` filter inclusion.
  • Snapshot serializer — strips SAML query params (`SAMLRequest`, `SAMLResponse`, `RelayState`), the `stack-saml-inner-` CSRF cookie prefix, and SAML xs:ID identifiers + IssueInstant/NotBefore/NotOnOrAfter timestamps so e2e snapshots don't reroll.
  • Seed-dummy-data — `STACK_SEED_ENABLE_SAML=true` block that pre-creates acme + globex SAML connections on the dummy project, fetching the IdP cert from the running mock at seed time. Dormant unless the env var is set; safe to ship before the backend SAML routes exist.

Why a separate service (and not just a stub in the test file)

The mock uses `samlify` deliberately because the upcoming backend SAML wrapper uses `@node-saml/node-saml`. Different libraries on each side mean a bug in either library's signature canonicalization surfaces as a test failure rather than canceling out (the backend would otherwise be its own test oracle, which is no oracle at all).

Endpoints

  • `GET /idp/:tenant/metadata` — IdP metadata XML
  • `GET /idp/:tenant/sso` — AuthnRequest receiver, renders a login form
  • `POST /idp/:tenant/login` — builds + auto-POSTs a signed SAMLResponse
  • `POST /idp/:tenant/test-controls` — queues misbehaviors for the next assertion: `bad-signature`, `expired`, `not-yet-valid`, `wrong-audience`, `wrong-in-response-to`, `missing-name-id`, `missing-email`, `replay`, `sign-with-tenant`. The backend's negative e2e tests trigger these to ensure the validator actually rejects bad assertions.
  • `GET /idp` — introspection (lists tenants + queued misbehaviors)

Test plan

  • `pnpm --filter @stackframe/mock-saml-idp typecheck` passes
  • `pnpm --filter @stackframe/mock-saml-idp lint` passes
  • `pnpm --filter @stackframe/mock-saml-idp start` boots and serves valid metadata for both tenants with distinct certs
  • CI `background-action` step starts the mock and `http://localhost:8115/idp\` becomes ready

The mock IdP is independently useful — it can back SAML testing for any future feature, not just the SSO work in the next PR.

Summary by CodeRabbit

Release Notes

  • New Features

    • Added a mock SAML 2.0 Identity Provider service supporting multiple tenants, per-tenant key generation, and configurable assertion scenarios.
  • Tests

    • Enhanced E2E test infrastructure with improved snapshot serialization, including better SAML authentication data redaction and parameter handling.
  • Chores

    • Integrated mock SAML IdP into the development launchpad for convenient access.

BilalG1 added 3 commits April 29, 2026 16:38
Adds apps/mock-saml-idp, a multi-tenant SAML 2.0 Identity Provider mock
mirroring apps/mock-oauth-server. Each tenant has its own RSA keypair
and self-signed cert generated at startup, so one mock service can back
many SamlConnection rows in tests and exercise per-connection isolation.

Uses samlify deliberately because the upcoming backend SAML wrapper will
use @node-saml/node-saml. Different libraries on each side means a bug
in either library's signature canonicalization surfaces as a test
failure instead of being masked by both sides agreeing.

Endpoints:
- GET  /idp/:tenant/metadata        IdP metadata XML
- GET  /idp/:tenant/sso             AuthnRequest receiver, renders login form
- POST /idp/:tenant/login           builds and auto-POSTs signed assertion
- POST /idp/:tenant/test-controls   queues misbehaviors (bad-signature,
                                    expired, wrong-audience, replay, etc.)
- GET  /idp                         introspection

Also adds @node-saml/node-saml to apps/backend deps for the upcoming
backend SAML protocol wrapper.
Three smaller pieces that unlock e2e testing:

- .github/workflows/e2e-api-tests.yaml: starts mock-saml-idp on port
  8115 alongside mock-oauth-server, with /idp as the readiness probe.
  Root package.json adds start:mock-saml-idp script and includes the
  mock in dev:basic.

- apps/e2e/tests/snapshot-serializer.ts: strips SAMLRequest /
  SAMLResponse / RelayState query+form params, adds stack-saml-inner-
  to keyed cookie name prefixes (so the per-AuthnRequest CSRF cookie
  doesn't reroll snapshots), and adds regex replacements for SAML xs:ID
  identifiers and IssueInstant/NotBefore/NotOnOrAfter timestamps.

- apps/backend/src/lib/seed-dummy-data.ts: STACK_SEED_ENABLE_SAML=true
  pre-creates acme + globex SAML connections on the dummy project,
  fetching the IdP metadata from the running mock at seed time so the
  seeded cert matches what the mock generated at startup. The mock
  regenerates keys per restart, so re-seed if you restart it. Mock URL
  configurable via STACK_MOCK_SAML_URL (default localhost:8115).
Deep dot-keys like `auth.saml.connections.X.field` get dropped by
config normalization with onDotIntoNonObject=ignore when the parent
record entry doesn't yet exist. Match the existing convention from
auth.oauth.providers and write the whole connection entry as a
single value.

(Bug surfaced when running the SAML e2e tests against a live
backend in a separate PR. Applied here so the seed function works
on its own without requiring downstream PRs.)
@vercel
Copy link
Copy Markdown

vercel Bot commented Apr 29, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
stack-auth-hosted-components Ready Ready Preview, Comment Apr 30, 2026 9:57pm
stack-backend Ready Ready Preview, Comment Apr 30, 2026 9:57pm
stack-dashboard Ready Ready Preview, Comment Apr 30, 2026 9:57pm
stack-demo Ready Ready Preview, Comment Apr 30, 2026 9:57pm
stack-docs Ready Ready Preview, Comment Apr 30, 2026 9:57pm
stack-preview-backend Ready Ready Preview, Comment Apr 30, 2026 9:57pm
stack-preview-dashboard Ready Ready Preview, Comment Apr 30, 2026 9:57pm

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 29, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9655e7f5-a505-47a6-9341-4903cd8fd46f

📥 Commits

Reviewing files that changed from the base of the PR and between 7dacbc4 and 6b8cd7e.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (6)
  • .github/workflows/e2e-api-tests.yaml
  • apps/dev-launchpad/public/index.html
  • apps/mock-saml-idp/.eslintrc.cjs
  • apps/mock-saml-idp/package.json
  • apps/mock-saml-idp/src/index.ts
  • package.json
✅ Files skipped from review due to trivial changes (3)
  • apps/mock-saml-idp/.eslintrc.cjs
  • apps/mock-saml-idp/package.json
  • apps/mock-saml-idp/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • .github/workflows/e2e-api-tests.yaml
  • package.json

📝 Walkthrough

Walkthrough

This pull request introduces a new mock SAML 2.0 Identity Provider service with multi-tenant support. It includes a standalone Express-based IDP server, updates the E2E workflow to launch it with health checks, extends snapshot serialization to redact SAML-specific data, and integrates the service into development tooling.

Changes

Cohort / File(s) Summary
Mock SAML IDP Application
apps/mock-saml-idp/package.json, apps/mock-saml-idp/src/index.ts, apps/mock-saml-idp/tsconfig.json, apps/mock-saml-idp/.eslintrc.cjs
New standalone SAML 2.0 IdP service with multi-tenant support, RSA keypair generation, metadata endpoints, SSO login flow, signed SAML response generation, misbehavior injection for testing, and replay mode. Includes TypeScript configuration, ESLint config, and dependency declarations.
Workflow & Testing Integration
.github/workflows/e2e-api-tests.yaml, apps/e2e/tests/snapshot-serializer.ts
E2E workflow now launches the mock SAML IDP with health check on port 8142; snapshot serializer redacts SAML query parameters, internal cookies, and XML timestamps for deterministic test outputs.
Root Configuration & Dev Launchpad
package.json, apps/dev-launchpad/public/index.html
Root package.json adds start:mock-saml-idp script, includes IDP in dev:basic filter, and extends KMS port-kill range; dev-launchpad UI adds card for SAML mock IdP service.

Sequence Diagram

sequenceDiagram
    participant Client
    participant MockSAMLIdP as Mock SAML IdP
    participant TestSuite as E2E Test Suite

    Client->>MockSAMLIdP: GET /idp/:tenant/sso?SAMLRequest=...
    MockSAMLIdP->>MockSAMLIdP: Decode SAMLRequest
    MockSAMLIdP-->>Client: Render login form (HTML)
    
    Client->>MockSAMLIdP: POST /idp/:tenant/login (credentials)
    MockSAMLIdP->>MockSAMLIdP: Check for queued misbehavior
    alt Misbehavior Active
        MockSAMLIdP->>MockSAMLIdP: Apply assertion modifications (e.g., expired)
    end
    MockSAMLIdP->>MockSAMLIdP: Generate signed SAML Response
    MockSAMLIdP-->>Client: POST form with SAML Response
    
    Client->>TestSuite: Submit SAML Response
    TestSuite->>TestSuite: Verify assertion (timestamp, audience, etc.)
    
    Client->>MockSAMLIdP: GET /idp/:tenant/test-controls (enqueue misbehavior)
    MockSAMLIdP->>MockSAMLIdP: Queue next assertion modification
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested reviewers

  • mantrakp04
  • nams1570

Poem

🐰 A SAML service hops into place,
Multi-tenant login flows embrace,
With mocked responses signed just right,
E2E tests can run through the night! 🔐

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 27.78% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the primary change: introducing a mock SAML 2.0 Identity Provider service for testing, which is the main focus of the entire changeset.
Description check ✅ Passed The description is comprehensive and well-structured, covering what the PR adds, endpoints, rationale, test plan, and context as a stacked PR. It exceeds the template requirements.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch pr/saml-mock-idp

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.

Comment @coderabbitai help to get the list of available commands and usage tips.

…ml-idp

mock-saml-idp originally depended on body-parser for the parser
middleware, but switched to using express.urlencoded()/express.json()
directly. The package.json dep was removed but the lockfile entry
remained, breaking 'pnpm install --frozen-lockfile' in CI.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
apps/backend/src/lib/seed-dummy-data.ts (1)

2072-2134: ⚠️ Potential issue | 🟠 Major

Avoid parallel environment override writes for the same project/branch.

seedSamlConnections(projectId) and the direct overrideEnvironmentConfigOverride call both run in parallel within Promise.all at lines 2127–2134. Both perform read-modify-write operations on the same environment config override. The implementation (line 491) already has a TODO acknowledging this: "put this in a serializable transaction (or a single SQL query) to prevent race conditions". When SAML is enabled, concurrent reads of the environment config can each see the stale state, apply their respective changes, and the second write will clobber the first—losing either the SAML config or the payments.testMode flag.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/backend/src/lib/seed-dummy-data.ts` around lines 2072 - 2134, Concurrent
environment writes in the Promise.all block can race: calls to
seedSamlConnections(projectId) and overrideEnvironmentConfigOverride both
perform read-modify-write on the same environment override; instead serialize
them by removing seedSamlConnections(projectId) from the parallel array and
await it before (or after) calling overrideEnvironmentConfigOverride, or merge
the SAML-related changes into the single overrideEnvironmentConfigOverride call
so only one read-modify-write occurs; update the Promise.all usage around
Promise.all([...overrideBranchConfigOverride(...), /* remove seedSamlConnections
from here */ overrideEnvironmentConfigOverride(...), ...]) and call await
seedSamlConnections(projectId) in sequence to prevent clobbering.
🧹 Nitpick comments (2)
apps/mock-saml-idp/src/index.ts (1)

88-94: Encode tenant slugs when generating mock IdP URLs.

Slugs from STACK_MOCK_SAML_TENANTS are directly interpolated into URLs. If a slug contains reserved URL characters like /, ?, or #, the metadata and SSO URLs will be malformed and tenant routing will fail. Per coding guidelines, use encodeURIComponent() to encode the path segment.

♻️ Suggested change
 function entityIdFor(slug: string): string {
-  return `http://localhost:${port}/idp/${slug}/metadata`;
+  const encodedSlug = encodeURIComponent(slug);
+  return `http://localhost:${port}/idp/${encodedSlug}/metadata`;
 }
 
 function ssoUrlFor(slug: string): string {
-  return `http://localhost:${port}/idp/${slug}/sso`;
+  const encodedSlug = encodeURIComponent(slug);
+  return `http://localhost:${port}/idp/${encodedSlug}/sso`;
 }

Also applies to line 432.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/mock-saml-idp/src/index.ts` around lines 88 - 94, The entityIdFor and
ssoUrlFor functions currently interpolate raw slug values into URLs, which
breaks when slugs contain reserved URL characters; update both functions (and
the similar occurrence around the symbol referenced at line ~432) to encode the
slug using encodeURIComponent() before inserting it into the path so metadata
and SSO URLs are always valid and safe for routing.
apps/backend/src/lib/seed-dummy-data.ts (1)

1986-2002: ⚡ Quick win

Remove the cast by typing overlay with the target parameter type.

The as Parameters<...> cast bypasses type safety and is avoidable here.

Suggested change
-  const overlay: Record<string, unknown> = {};
+  const overlay: Parameters<typeof overrideEnvironmentConfigOverride>[0]['environmentConfigOverrideOverride'] = {};
@@
   await overrideEnvironmentConfigOverride({
     projectId,
     branchId: DEFAULT_BRANCH_ID,
-    environmentConfigOverrideOverride: overlay as Parameters<typeof overrideEnvironmentConfigOverride>[0]["environmentConfigOverrideOverride"],
+    environmentConfigOverrideOverride: overlay,
   });

As per coding guidelines, "Do NOT use as/any/type casts or anything else like that to bypass the type system."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/backend/src/lib/seed-dummy-data.ts` around lines 1986 - 2002, The
overlay object is being cast with a broad as-cast when passed to
overrideEnvironmentConfigOverride; instead declare overlay with the exact
parameter type so you don’t bypass the type system. Replace the current const
overlay: Record<string, unknown> = {} with a declaration using Parameters<typeof
overrideEnvironmentConfigOverride>[0]["environmentConfigOverrideOverride"] as
the type (e.g., const overlay: Parameters<typeof
overrideEnvironmentConfigOverride>[0]["environmentConfigOverrideOverride"] =
{}), keep building properties from fetched (f.slug, f.displayName, etc.) and
then call overrideEnvironmentConfigOverride(projectId, DEFAULT_BRANCH_ID, {
environmentConfigOverrideOverride: overlay }) without the as cast. Ensure the
property shapes you assign match that parameter type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/backend/src/lib/seed-dummy-data.ts`:
- Around line 1958-1961: The fetch to `${mockUrl}/idp/${t.slug}/metadata` can
hang and uses an unescaped slug; update the call in seed-dummy-data (where
`mockUrl`, `t.slug` and `fetch` are used) to build the URL safely with
encodeURIComponent(t.slug) (or urlString`` helper) and wrap the fetch with an
AbortController timeout: create an AbortController, pass controller.signal to
fetch, set a timer to controller.abort() after a short bound (e.g. 5s), and
clear the timer after the response; ensure the existing error handling still
throws when res.ok is false and that aborted fetches surface as errors.

In `@apps/e2e/tests/snapshot-serializer.ts`:
- Around line 116-121: The snapshots still include POST-bound SAML hidden inputs
because only stripUrlQueryParams is removing those keys from URLs; update the
snapshot serializer to also redact form input values named "SAMLRequest",
"SAMLResponse", and "RelayState" when serializing HTML forms: extend the
existing HTML/form sanitization logic (where stripUrlQueryParams is used) to
detect <input> elements (hidden or otherwise) with those name attributes and
replace their value with a stable placeholder, and apply the same change to the
other similar block referenced around the second occurrence (the lines analogous
to 134-140) so both URL and form payloads are sanitized consistently.

In `@apps/mock-saml-idp/package.json`:
- Around line 14-24: The lockfile wasn't updated to reflect the new workspace
manifest, leaving the old specifier for body-parser@^1.20.3 in pnpm-lock.yaml
and breaking CI; run a root workspace install (pnpm install or pnpm -w install)
so pnpm regenerates pnpm-lock.yaml with the correct specifiers for the new
mock-saml-idp package, verify pnpm-lock.yaml now reflects the updated
dependencies, and commit the updated pnpm-lock.yaml alongside the package.json
change.

In `@apps/mock-saml-idp/src/index.ts`:
- Around line 417-423: The handler currently casts req.body to Misbehavior and
assigns it to t.nextMisbehavior without validating fields, which lets invalid
payloads through; update the /test-controls handler in index.ts to explicitly
validate that req.body.kind is one of the allowed Misbehavior discriminants
(e.g., the union of valid string kinds), then perform per-kind validation of
required fields for that variant (reject null or missing fields), avoid using
`as`/any casts, and only assign a properly narrowed/validated object to
t.nextMisbehavior; on validation failure return res.status(400).json({ error:
"..." }) with a clear message.

---

Outside diff comments:
In `@apps/backend/src/lib/seed-dummy-data.ts`:
- Around line 2072-2134: Concurrent environment writes in the Promise.all block
can race: calls to seedSamlConnections(projectId) and
overrideEnvironmentConfigOverride both perform read-modify-write on the same
environment override; instead serialize them by removing
seedSamlConnections(projectId) from the parallel array and await it before (or
after) calling overrideEnvironmentConfigOverride, or merge the SAML-related
changes into the single overrideEnvironmentConfigOverride call so only one
read-modify-write occurs; update the Promise.all usage around
Promise.all([...overrideBranchConfigOverride(...), /* remove seedSamlConnections
from here */ overrideEnvironmentConfigOverride(...), ...]) and call await
seedSamlConnections(projectId) in sequence to prevent clobbering.

---

Nitpick comments:
In `@apps/backend/src/lib/seed-dummy-data.ts`:
- Around line 1986-2002: The overlay object is being cast with a broad as-cast
when passed to overrideEnvironmentConfigOverride; instead declare overlay with
the exact parameter type so you don’t bypass the type system. Replace the
current const overlay: Record<string, unknown> = {} with a declaration using
Parameters<typeof
overrideEnvironmentConfigOverride>[0]["environmentConfigOverrideOverride"] as
the type (e.g., const overlay: Parameters<typeof
overrideEnvironmentConfigOverride>[0]["environmentConfigOverrideOverride"] =
{}), keep building properties from fetched (f.slug, f.displayName, etc.) and
then call overrideEnvironmentConfigOverride(projectId, DEFAULT_BRANCH_ID, {
environmentConfigOverrideOverride: overlay }) without the as cast. Ensure the
property shapes you assign match that parameter type.

In `@apps/mock-saml-idp/src/index.ts`:
- Around line 88-94: The entityIdFor and ssoUrlFor functions currently
interpolate raw slug values into URLs, which breaks when slugs contain reserved
URL characters; update both functions (and the similar occurrence around the
symbol referenced at line ~432) to encode the slug using encodeURIComponent()
before inserting it into the path so metadata and SSO URLs are always valid and
safe for routing.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e34fa459-9761-47db-ab1d-53af2b88916a

📥 Commits

Reviewing files that changed from the base of the PR and between e831972 and 4949a9c.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (9)
  • .github/workflows/e2e-api-tests.yaml
  • apps/backend/package.json
  • apps/backend/src/lib/seed-dummy-data.ts
  • apps/e2e/tests/snapshot-serializer.ts
  • apps/mock-saml-idp/.eslintrc.js
  • apps/mock-saml-idp/package.json
  • apps/mock-saml-idp/src/index.ts
  • apps/mock-saml-idp/tsconfig.json
  • package.json

Comment thread apps/backend/src/lib/seed-dummy-data.ts Outdated
Comment thread apps/e2e/tests/snapshot-serializer.ts
Comment thread apps/mock-saml-idp/package.json
Comment thread apps/mock-saml-idp/src/index.ts
The pattern \`/_[a-zA-Z][a-zA-Z0-9_.-]{8,}/g\` matched any SCREAMING_SNAKE_CASE
identifier with an underscore followed by 9+ chars — e.g.
\`STRIPE_ACCOUNT_INFO_NOT_FOUND\` became \`STRIPE<stripped SAML id>\`,
breaking unrelated Stripe / payments / OAuth e2e snapshots.

The regex isn't load-bearing today: no current SAML test snapshots a
random AuthnRequest / Response / Assertion ID. Cookie-name SAML IDs
are already covered by \`keyedCookieNamePrefixes\` ("stack-saml-inner-"),
URL path segments only carry the deterministic connection_id, and no
test snapshots raw SAML XML. If a future test ever does, follow the
precedent of the timestamp strip on the next line and anchor the
replacement to specific XML attributes (\`ID="..."\`, \`InResponseTo="..."\`)
rather than matching loose \`_<chars>\` strings everywhere.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
apps/e2e/tests/snapshot-serializer.ts (1)

116-121: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

POST-bound SAML form payloads are still not redacted.

stripUrlQueryParams only applies inside URL parsing (Line 201), so hidden form inputs like SAMLResponse/RelayState remain dynamic and can still reroll snapshots.

Suggested fix
 const stringRegexReplacements = [
   [/(\/integrations\/(neon|custom)\/oauth\/idp\/(interaction|auth)\/)[a-zA-Z0-9_-]+/gi, "$1<stripped $3 UID>"],
   [new RegExp(`localhost\:${getPortPrefix()}`, "gi"), "localhost:<$$NEXT_PUBLIC_STACK_PORT_PREFIX>"],
   [new RegExp(`localhost\%3A${getPortPrefix()}`, "gi"), "localhost%3A%3C%24NEXT_PUBLIC_STACK_PORT_PREFIX%3E"],
   [/(Timeout exceeded: elapsed )[0-9.]+( ms)/gi, "$1<stripped time>$2"],
+  [/(name="(?:SAMLRequest|SAMLResponse|RelayState)"\s+value=")[^"]*(")/g, '$1<stripped SAML form value>$2'],
   // SAML XML timestamps (e.g. IssueInstant, NotBefore, NotOnOrAfter).
   [/(IssueInstant|NotBefore|NotOnOrAfter)="[^"]+"/g, '$1="<stripped SAML timestamp>"'],
 ] as const;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/e2e/tests/snapshot-serializer.ts` around lines 116 - 121, The serializer
currently redacts SAML params only from URLs via stripUrlQueryParams, leaving
POST-bound hidden form fields like SAMLResponse and RelayState dynamic; update
the snapshot serializer to also detect and replace values of hidden form inputs
(names "SAMLRequest", "SAMLResponse", "RelayState") during HTML serialization so
snapshots are stable. Locate the HTML form handling/serialization logic in this
file (where stripUrlQueryParams is defined/used) and add a step that parses form
input elements (or runs a regex over <input ... type="hidden" name="...">) to
replace their value attributes with a fixed placeholder, applying the same
redaction list already used for URLs. Ensure the change targets those exact
parameter names so other inputs remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@apps/e2e/tests/snapshot-serializer.ts`:
- Around line 116-121: The serializer currently redacts SAML params only from
URLs via stripUrlQueryParams, leaving POST-bound hidden form fields like
SAMLResponse and RelayState dynamic; update the snapshot serializer to also
detect and replace values of hidden form inputs (names "SAMLRequest",
"SAMLResponse", "RelayState") during HTML serialization so snapshots are stable.
Locate the HTML form handling/serialization logic in this file (where
stripUrlQueryParams is defined/used) and add a step that parses form input
elements (or runs a regex over <input ... type="hidden" name="...">) to replace
their value attributes with a fixed placeholder, applying the same redaction
list already used for URLs. Ensure the change targets those exact parameter
names so other inputs remain unchanged.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 405857f8-f34a-440a-9a44-bf90befa0e6d

📥 Commits

Reviewing files that changed from the base of the PR and between 4949a9c and aaeb831.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (1)
  • apps/e2e/tests/snapshot-serializer.ts

Two cleanups in seed-dummy-data, both flagged on PR #1395:

- The parallel `Promise.all` block ran `seedSamlConnections(projectId)`
  alongside another `overrideEnvironmentConfigOverride` write on the same
  project (`payments.testMode`). Both are read-modify-write, so concurrent
  reads of the env config can each see the stale state and the second
  write clobbers the first — the existing TODO at `config.ts:491` already
  documents the underlying race. Sequence the SAML seed after the
  parallel block to avoid the race until the override is wrapped in a
  serializable txn.

- Type the SAML overlay with the target parameter type directly instead
  of using an `as Parameters<...>` cast, per project style.
`${keyedCookieNamePrefixes}` interpolated the entire array, which was
harmless when the array had one entry but produced
"stack-oauth-inner-,stack-saml-inner-" once a second prefix was added,
breaking every existing OAuth set-cookie snapshot.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
apps/e2e/tests/snapshot-serializer.ts (1)

116-121: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

POST-bound SAML form values are still not redacted.

stripUrlQueryParams only helps when SAML fields appear in URLs. The mock IdP auto-post page carries SAMLRequest/SAMLResponse/RelayState in hidden form inputs, so snapshots can still churn.

Suggested patch
 const stringRegexReplacements = [
   [/(\/integrations\/(neon|custom)\/oauth\/idp\/(interaction|auth)\/)[a-zA-Z0-9_-]+/gi, "$1<stripped $3 UID>"],
   [new RegExp(`localhost\:${getPortPrefix()}`, "gi"), "localhost:<$$NEXT_PUBLIC_STACK_PORT_PREFIX>"],
   [new RegExp(`localhost\%3A${getPortPrefix()}`, "gi"), "localhost%3A%3C%24NEXT_PUBLIC_STACK_PORT_PREFIX%3E"],
   [/(Timeout exceeded: elapsed )[0-9.]+( ms)/gi, "$1<stripped time>$2"],
+  [/(name="(?:SAMLRequest|SAMLResponse|RelayState)"\s+value=")[^"]*(")/g, '$1<stripped SAML form value>$2'],
   [/(IssueInstant|NotBefore|NotOnOrAfter)="[^"]+"/g, '$1="<stripped SAML timestamp>"'],
 ] as const;

Also applies to: 134-135

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/e2e/tests/snapshot-serializer.ts` around lines 116 - 121, The SAML form
hidden inputs (names "SAMLRequest", "SAMLResponse", "RelayState") are not being
redacted because stripUrlQueryParams only handles URLs; update the snapshot
serializer that handles HTML output (the same code that currently calls
stripUrlQueryParams) to also scan form inputs and replace value attributes for
inputs whose name matches "SAMLRequest" | "SAMLResponse" | "RelayState" with a
stable placeholder (same placeholder used for URL params), handling
single/double quotes and variations (hidden/text/textarea), and apply the same
change for the other occurrence referenced around lines 134-135 so POST-bound
SAML values are consistently redacted from snapshots.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@apps/e2e/tests/snapshot-serializer.ts`:
- Around line 116-121: The SAML form hidden inputs (names "SAMLRequest",
"SAMLResponse", "RelayState") are not being redacted because stripUrlQueryParams
only handles URLs; update the snapshot serializer that handles HTML output (the
same code that currently calls stripUrlQueryParams) to also scan form inputs and
replace value attributes for inputs whose name matches "SAMLRequest" |
"SAMLResponse" | "RelayState" with a stable placeholder (same placeholder used
for URL params), handling single/double quotes and variations
(hidden/text/textarea), and apply the same change for the other occurrence
referenced around lines 134-135 so POST-bound SAML values are consistently
redacted from snapshots.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4efbfe4e-a8de-4328-b6fc-896d7ea762f8

📥 Commits

Reviewing files that changed from the base of the PR and between 1e912c7 and 7dacbc4.

📒 Files selected for processing (1)
  • apps/e2e/tests/snapshot-serializer.ts

@mantrakp04
Copy link
Copy Markdown
Collaborator

@greptile-ai review

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 30, 2026

Greptile Summary

This PR introduces a standalone mock SAML 2.0 Identity Provider (apps/mock-saml-idp) built with Express + samlify, wired into CI and dev:basic. It supports multi-tenant key generation, configurable misbehaviors for negative e2e tests, and includes a correct bug-fix in the snapshot serializer where the cookie-name stripping branch was using the entire prefix array as a string instead of the matched prefix.

Confidence Score: 5/5

Safe to merge — all findings are P2 style/quality issues with no impact on test correctness or production systems.

No P0 or P1 defects found. The two P2 notes (wrong @types/express major version, stale tsconfig template settings) don't affect runtime behavior since typecheck reportedly passes. The snapshot-serializer cookie fix is a genuine improvement.

apps/mock-saml-idp/package.json (@types/express version) and apps/mock-saml-idp/tsconfig.json (Next.js-derived lib/jsx settings).

Important Files Changed

Filename Overview
apps/mock-saml-idp/src/index.ts New multi-tenant mock SAML IdP service; well-structured with correct Map usage, but user email is inserted into XML without entity-escaping and the test-controls endpoint doesn't validate kind against the known union values.
apps/mock-saml-idp/package.json New package; @types/express@5 declared against express@4 runtime — type/runtime version mismatch. Type-only packages correctly placed in devDependencies.
apps/mock-saml-idp/tsconfig.json Copied from a Next.js template; includes unnecessary dom lib and jsx: preserve settings for a plain Node.js Express server, allowing browser-only API usage to pass type checks.
apps/e2e/tests/snapshot-serializer.ts Adds SAML query params and cookie prefix; also includes a correct bug-fix: the old keyedCookieNamePrefixes.some(…) branch used the entire array as a string instead of the matched prefix — now properly uses matchedPrefix.
.github/workflows/e2e-api-tests.yaml Adds background-action step for mock-saml-idp on port 8142; correctly matches the default port computed in src/index.ts.
apps/mock-saml-idp/.eslintrc.cjs Correctly extends defaults.js (not next.js), appropriate for a plain Express service.

Sequence Diagram

sequenceDiagram
    participant SP as Service Provider (Backend)
    participant IdP as Mock SAML IdP (:8142)
    participant Browser

    SP->>Browser: Redirect to /idp/:tenant/sso?SAMLRequest=...
    Browser->>IdP: GET /idp/:tenant/sso?SAMLRequest=&RelayState=
    IdP->>IdP: parseAuthnRequestRedirect (inflate + extract fields)
    IdP->>Browser: Render login form (email input, hidden SAMLRequest)

    opt E2E test misbehavior
        Browser->>IdP: POST /idp/:tenant/test-controls {kind: bad-signature|expired|...}
        IdP->>IdP: Set nextMisbehavior for tenant
    end

    Browser->>IdP: POST /idp/:tenant/login (email, SAMLRequest, RelayState)
    IdP->>IdP: consumeNextMisbehavior → buildAssertion → renderLoginResponseXml
    IdP->>IdP: samlify.createLoginResponse (signs + base64-encodes)
    IdP->>Browser: Render auto-POST form (SAMLResponse, RelayState, ACS URL)
    Browser->>SP: POST ACS URL (SAMLResponse=..., RelayState=...)
    SP->>SP: Validate assertion (via @node-saml/node-saml)
Loading
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 3
apps/mock-saml-idp/package.json:120
**`@types/express` v5 types used with Express v4 runtime**

`express` is pinned to `^4.21.2` (Express 4), but `@types/express` is `^5.0.0` (Express 5 types). The two major versions have different API signatures — for example, Express 5's `req.query` and async error-handling differ from v4. Using v5 type declarations against a v4 runtime means TypeScript may accept code that behaves differently at runtime, or flag valid v4 patterns as errors.

```suggestion
    "@types/express": "^4.17.21",
```

### Issue 2 of 3
apps/mock-saml-idp/tsconfig.json:645-651
**`dom` lib and `jsx` setting inappropriate for a Node.js Express server**

This `tsconfig.json` is copied from a Next.js template. `"lib": ["dom", "dom.iterable", "esnext"]` exposes browser-global types (`window`, `document`, etc.) to a Node.js server, meaning TypeScript won't catch accidental use of browser-only APIs. `"jsx": "preserve"` is irrelevant in a plain Express service with no JSX. Consider using `"lib": ["esnext"]` and removing the `"jsx"` field entirely.

### Issue 3 of 3
apps/mock-saml-idp/src/index.ts:421-430
**User-supplied email embedded verbatim into SAML XML without escaping**

`user.email` (submitted by the browser via the login form) is inserted directly into the XML response template via string replacement. An email containing XML meta-characters such as `<`, `>`, `"`, or `&` (e.g. `alice@test.com&foo=bar`) will produce malformed XML that samlify signs but the SP parser may reject with an opaque parse error. For a test mock the input is controlled, but it makes failure modes hard to diagnose. Consider HTML-encoding the value before substitution.

Reviews (2): Last reviewed commit: "chore(backend): defer SAML seed + node-s..." | Re-trigger Greptile

Comment thread apps/mock-saml-idp/package.json Outdated
Comment thread apps/mock-saml-idp/.eslintrc.cjs
Comment thread apps/mock-saml-idp/src/index.ts
Comment thread apps/backend/src/lib/seed-dummy-data.ts Outdated
Comment thread apps/backend/src/lib/seed-dummy-data.ts Outdated
Comment thread apps/mock-saml-idp/src/index.ts Outdated
Comment thread apps/backend/package.json Outdated
Comment thread apps/mock-saml-idp/src/index.ts Outdated
BilalG1 added 4 commits April 30, 2026 14:44
- Move @types/express and @types/node-forge to devDependencies — they're
  compile-time only.
- Drop the next.js ESLint extend (this is a plain Express service, not Next).
  Rename .eslintrc.js to .eslintrc.cjs to match the convention used by every
  other workspace package.
- Add --ext .tsx,.ts to the lint script (required when only the defaults
  config is used, since the bare typescript-eslint parser doesn't pick up
  .ts/.tsx by default; matches apps/e2e and packages/stack-shared).
The mock SAML IdP previously used port suffix 15, which is also bound by
examples/supabase. Under \`pnpm dev:basic\` / \`dev:full\` whichever started
second failed to bind. Suffixes 01–41 are otherwise spoken for; 42 is the
first free slot before the LocalStack reservation at 50–99.

- Default port → \`\${prefix}42\`
- \`pnpm kms\` cleanup list grows to include 42
- e2e CI health-check URL updated to \`http://localhost:8142/idp\`
- Add a dev launchpad tile so the SAML mock is discoverable next to the
  OAuth mock
Two related changes in apps/mock-saml-idp/src/index.ts:

1. Replay misbehavior now re-emits the original RelayState alongside the
   cached SAMLResponse. Previously buildAssertion returned the cached
   SAMLResponse but the login handler still rendered the form with the
   *current* request's RelayState, which tests "old response + fresh
   state" rather than a true replay. AssertionResult now carries
   relayState so the handler uses whatever the assertion path returned.

2. Split the 100-line buildAssertion into focused helpers:
   consumeNextMisbehavior, resolveSigningTenant, buildAssertionFields,
   renderLoginResponseXml, cacheReplayableResponse. Same behavior; the
   replay short-circuit and the cache update are now obvious at a glance.

Also updates the file header to clarify that the @node-saml/node-saml
dependency it pairs with lands in the stacked backend PR — this PR
ships the mock alone.
Removes seedSamlConnections (and its STACK_SEED_ENABLE_SAML callsite)
plus the @node-saml/node-saml dependency from this PR. Both depend on
config.auth.saml schema entries that don't exist on this branch yet —
the seed wrote overrides that were silently dropped during config
normalization, and node-saml had no consumer here.

They land together in the stacked backend PR alongside the schema and
the SAML protocol wrapper that actually imports node-saml.
@github-actions github-actions Bot assigned BilalG1 and unassigned mantrakp04 May 1, 2026
@BilalG1
Copy link
Copy Markdown
Collaborator Author

BilalG1 commented May 4, 2026

closing for now, handling this post launch

@BilalG1 BilalG1 closed this May 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants